Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

migrate majority of tests to junit5 #3711

Merged
merged 8 commits into from
Feb 11, 2018
Merged

migrate majority of tests to junit5 #3711

merged 8 commits into from
Feb 11, 2018

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Feb 7, 2018

please check if I didn't forget one Test annotation


  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

@@ -31,15 +31,14 @@ public void testExistenceOfIconImagesReferencedFromIconsProperties() throws IOEx
try (Reader reader = Files.newBufferedReader(Paths.get(iconsPropertiesPath))) {
properties.load(reader);
}
assertFalse("There must be loaded properties after loading " + iconsPropertiesPath,
properties.entrySet().isEmpty());
assertFalse(properties.entrySet().isEmpty(), () -> "There must be loaded properties after loading " + iconsPropertiesPath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, not sure if I like these Supplier Lambdas...Are they really needed? Makes the methods look more complex and I doubt that the evaluation of the message really takes a long time. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the beginning I did not see that I can pass the message as parameter without a supplier . The later tests are without. The supplier is only needed in the assertThroes

remove unnecessary supplier
convert some more tests to junit5
fix imports
Mock externalFileTypes in importer test
Convert FormatterTest to junit5
convert architecture test to junit5
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 10, 2018
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I scrolled over the code and it looks good to me (there are of course too much changes to verify everything in detail). Since the tests passes and the code coverage stays almost the same, everything should work as expected.

@Siedlerchr
Copy link
Member Author

Siedlerchr commented Feb 11, 2018

I am not sure If I should merge this or wait for #3704
@koppor Merge or wait for #3704 ?

@koppor koppor merged commit 1cef924 into master Feb 11, 2018
@koppor koppor deleted the junit5 branch February 11, 2018 16:06
@koppor
Copy link
Member

koppor commented Feb 11, 2018

I am still struggling with Install4J. Not sure, when I'll have one that fight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants